feat: UX enhancements in HR & Payroll navigation & list views#4212
feat: UX enhancements in HR & Payroll navigation & list views#4212krishna-254 wants to merge 2 commits intofrappe:developfrom
Conversation
…for employement_type
WalkthroughRenames navigation and UI labels from "People" to "Setup" across multiple JSON configuration files (desktop icon, workspace, workspace sidebar). Adds three dashboard items under Payroll: "Overtime Slip", "Arrear", and "Payroll Correction". Introduces a patch script that deletes existing "People" documents for doctypes "Workspace Sidebar", "Workspace", and "Desktop Icon" and clears the cache; registers the patch in hrms/patches.txt. Enables 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hrms/setup.py (1)
183-190:⚠️ Potential issue | 🟠 MajorBackfill this list-view change for upgraded sites.
If this feature is expected on existing sites too, this change alone only covers fresh installs:
get_custom_fields()is applied fromafter_install()on Line 15. Upgrades need a patch that updates the existingemployment_typecustom-field metadata and clears theEmployeecache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/setup.py` around lines 183 - 190, Add an upgrade patch (separate from after_install() where get_custom_fields() runs only on fresh installs) that updates the existing Employee custom field "employment_type" to match the new metadata (fieldtype Link, options "Employment Type", label _("Employment Type"), insert_after "department", in_list_view 1, ignore_user_permissions 1) and then clear the Employee doctype cache so changes take effect; locate the custom field by fieldname "employment_type" on doctype "Employee", update its properties, save it (or use the framework's update API), and call the cache-clearing routine for Employee so upgraded sites get the new list-view behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hrms/patches/v16_0/rename_people_to_setup.py`:
- Around line 4-9: Add the patch module
hrms.patches.v16_0.rename_people_to_setup to the post_model_sync list in
hrms/patches.txt so the execute() function in that module runs during migration;
open hrms/patches.txt and append "hrms.patches.v16_0.rename_people_to_setup"
(matching the module name) in the post_model_sync section of the file.
---
Outside diff comments:
In `@hrms/setup.py`:
- Around line 183-190: Add an upgrade patch (separate from after_install() where
get_custom_fields() runs only on fresh installs) that updates the existing
Employee custom field "employment_type" to match the new metadata (fieldtype
Link, options "Employment Type", label _("Employment Type"), insert_after
"department", in_list_view 1, ignore_user_permissions 1) and then clear the
Employee doctype cache so changes take effect; locate the custom field by
fieldname "employment_type" on doctype "Employee", update its properties, save
it (or use the framework's update API), and call the cache-clearing routine for
Employee so upgraded sites get the new list-view behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ffefc54-025f-4528-8331-8e4b6e69a9e5
⛔ Files ignored due to path filters (2)
hrms/public/icons/desktop_icons/setup.svgis excluded by!**/*.svghrms/public/icons/desktop_icons/solid/setup.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
hrms/desktop_icon/setup.jsonhrms/hr/workspace/setup/setup.jsonhrms/overrides/dashboard_overrides.pyhrms/patches/v16_0/rename_people_to_setup.pyhrms/setup.pyhrms/workspace_sidebar/setup.json
krishna-254
force-pushed
the
feat/employee-creation-and-lifecycle
branch
from
bce61d1 to
f84166c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hrms/patches/v16_0/rename_people_to_setup.py`:
- Around line 5-7: The current loop deletes any document named "People" which is
destructive; instead, detect only the shipped/global records named "People" and
rename them in-place to "Setup". In the loop over ("Workspace
Sidebar","Workspace","Desktop Icon") replace the frappe.delete_doc call with
logic that: 1) verifies the record exists and is a shipped/global record (e.g.
frappe.db.get_value(doctype, "People", ["owner", "is_custom"]) and ensure owner
== "Administrator" and is_custom is not truthy, or use any project-specific flag
that marks a standard record), and 2) calls frappe.rename_doc(doctype, "People",
"Setup", ignore_permissions=True, force=True) (or frappe.db.set_value/rename
equivalent) only when that check passes so site-specific or custom "People" rows
are not removed. Ensure you keep the existing loop and checks around
frappe.db.exists/frappe.delete_doc and replace the delete with the scoped
renaming logic.
- Around line 4-9: The migration currently deletes "People" in execute() for
doctypes ("Workspace Sidebar", "Workspace", "Desktop Icon") but does not ensure
the replacement "Setup" records are recreated on upgrade; inspect hrms/hooks.py
for fixtures or sync declarations covering hrms/hr/workspace/setup/setup.json,
hrms/workspace_sidebar/setup.json, and hrms/desktop_icon/setup.json and if
missing either add those files to the app's fixtures/sync path or add a
migration step that explicitly creates the three "Setup" records (for doctypes
Workspace Sidebar, Workspace, Desktop Icon) after deletion so upgraded sites
receive the navigation entries; reference the execute() function and the three
JSON files in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c31d923e-8762-4989-8512-bdee8e263900
⛔ Files ignored due to path filters (2)
hrms/public/icons/desktop_icons/setup.svgis excluded by!**/*.svghrms/public/icons/desktop_icons/solid/setup.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
hrms/desktop_icon/setup.jsonhrms/hr/workspace/setup/setup.jsonhrms/patches.txthrms/patches/v16_0/rename_people_to_setup.pyhrms/workspace_sidebar/setup.json
🚧 Files skipped from review as they are similar to previous changes (1)
- hrms/workspace_sidebar/setup.json
|
Approved, rebase and merge |
|
Isn’t “setup” going to conflict with other module setups as we discussed during the ux session? |
|
#4091 This still has that point mentioned, so I thought we agreed on the name |
|
I think we were looking for a way to name it HR Setup and keeping the title as Setup but framework doesn’t support it. Wasn’t captured in the notes |
|
Accounts has “Accounts Setup” 
|
ruchamahabal
changed the title
3 participants
Part of #4091
Changes
Summary by CodeRabbit
New Features
Chores